Reviewing Open Saas code#369
Conversation
| - Make sure you have the `.env.client` and `.env.server` files with correct dev values in the root of the project. | ||
| - Run the database with `wasp start db` and leave it running. | ||
| - Run `wasp start` and leave it running. | ||
| - [OPTIONAL]: If this is the first time starting the app, or you've just made changes to your entities/prisma schema, also run `wasp db migrate-dev`. |
There was a problem hiding this comment.
Optional is probably not the right word here, since it's mandatory when it's mandatory, and has no effect otherwise.
| if (!context.user) { | ||
| throw new HttpError(401); | ||
| } | ||
|
|
||
| if (!context.user.isAdmin) { | ||
| throw new HttpError(403); | ||
| } |
There was a problem hiding this comment.
Until we get a proper middleware, we should probably cook up our own decorators for this (or at least call a function that checks everything and throws.
| const updatedUser = await context.entities.User.update({ | ||
| where: { | ||
| id, | ||
| }, | ||
| data: { | ||
| isAdmin: data.isAdmin, | ||
| }, | ||
| }); | ||
|
|
||
| return updatedUser; |
There was a problem hiding this comment.
updatedUser can be returned directly, meaning that the await is redundant as well.
| return updatedUser; | ||
| }; | ||
|
|
||
| export const updateCurrentUserLastActiveTimestamp: UpdateCurrentUserLastActiveTimestamp<Pick<User, 'lastActiveTimestamp'>, User> = async ({ lastActiveTimestamp }, context) => { |
There was a problem hiding this comment.
We have plenty of long lines in this file (and other files). How come prettier doesn't take care of it? We should look into that
| const trigger = useRef<any>(null); | ||
| const dropdown = useRef<any>(null); | ||
|
|
||
| const toggleDropdown = () => setDropdownOpen((prev) => !prev); |
There was a problem hiding this comment.
I might be wrong, but I have a feeling React has a more native support for this (more precisely, I don't think we need this wrapper).
| const trigger = useRef<any>(null); | ||
| const dropdown = useRef<any>(null); |
There was a problem hiding this comment.
This is strange, both because of useRef (which should be used with cautionandany`.
| <span className='block text-sm font-medium dark:text-white'>{user.username}</span> | ||
| </span> | ||
| <CgProfile size='1.1rem' className='ml-1 mt-[0.1rem] dark:text-white' /> | ||
| <svg |
There was a problem hiding this comment.
Again , we should probably extract SVGs into their own files.
| ); | ||
| }; | ||
|
|
||
| export default DropdownUser; |
There was a problem hiding this comment.
I advise against using defualt exports.
There was a problem hiding this comment.
TODO: Continue tomorrow (stopped at DropdownUser)
sodic
left a comment
There was a problem hiding this comment.
Second pass, superficial review of most modules.
| export const paymentsWebhook = paymentProcessor.webhook; | ||
| export const paymentsMiddlewareConfigFn = paymentProcessor.webhookMiddlewareConfigFn; |
There was a problem hiding this comment.
This pattern looks weird, I should investigate what's going on here.
| @@ -0,0 +1,8 @@ | |||
| export function requireNodeEnvVar(name: string): string { | |||
There was a problem hiding this comment.
I think something like this is now supported through Wasp via Zod.
| const firstName = faker.person.firstName(); | ||
| const lastName = faker.person.lastName(); | ||
| const subscriptionStatus = faker.helpers.arrayElement<SubscriptionStatus | null>(['active', 'cancel_at_period_end', 'past_due', 'deleted', null]); | ||
| const now = new Date(); | ||
| const createdAt = faker.date.past({ refDate: now }); | ||
| const lastActiveTimestamp = faker.date.between({ from: createdAt, to: now }); | ||
| const credits = subscriptionStatus ? 0 : faker.number.int({ min: 0, max: 10 }); | ||
| const hasUserPaidOnStripe = !!subscriptionStatus || credits > 3 |
There was a problem hiding this comment.
Seems like some of these are redundant.
| export const DocsUrl = 'https://docs.opensaas.sh'; | ||
| export const BlogUrl = 'https://docs.opensaas.sh/blog'; |
There was a problem hiding this comment.
These should be in all caps or in camelCase.
| * For more info see: https://wasp.sh/docs/data-model/backends#seeding-the-database | ||
| */ | ||
| export async function seedMockUsers(prismaClient: PrismaClient) { | ||
| await Promise.all(generateMockUsersData(50).map((data) => |
There was a problem hiding this comment.
The await is redundant.
| const priorityOrder = ['low', 'medium', 'high']; | ||
| if (a.props.mainTask.priority && b.props.mainTask.priority) { | ||
| return ( | ||
| priorityOrder.indexOf(b.props.mainTask.priority) - priorityOrder.indexOf(a.props.mainTask.priority) | ||
| ); | ||
| } else { | ||
| return 0; | ||
| } | ||
| }) |
There was a problem hiding this comment.
This should probably be its own function.
| )} | ||
| </table> | ||
|
|
||
| {/* ))} */} |
| const shouldDisplayAppNavBar = useMemo(() => { | ||
| return location.pathname !== routes.LoginRoute.build() && location.pathname !== routes.SignupRoute.build(); | ||
| }, [location]); | ||
|
|
||
| const isAdminDashboard = useMemo(() => { | ||
| return location.pathname.startsWith('/admin'); | ||
| }, [location]); |
There was a problem hiding this comment.
Why useMemo, was there a performance hit?
| return location.pathname.startsWith('/admin'); | ||
| }, [location]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
We should name the useEffect.
| const lastSeenAt = new Date(user.lastActiveTimestamp); | ||
| const today = new Date(); | ||
| if (today.getTime() - lastSeenAt.getTime() > 5 * 60 * 1000) { | ||
| updateCurrentUserLastActiveTimestamp({ lastActiveTimestamp: today }); |
There was a problem hiding this comment.
As mentioned, the timestamp sould be calculated on the backend.
There was a problem hiding this comment.
I don't think we need this
|
|
||
| const Header = (props: { | ||
| sidebarOpen: string | boolean | undefined; | ||
| setSidebarOpen: (arg0: boolean) => void; |
There was a problem hiding this comment.
We should name the argument.
| // close on click outside | ||
| useEffect(() => { |
There was a problem hiding this comment.
We should name the useEffect.
Applies to other places too.
| const trigger = useRef<any>(null); | ||
| const sidebar = useRef<any>(null); |
There was a problem hiding this comment.
We should remove the anys.
| const trigger = useRef<any>(null); | ||
| const sidebar = useRef<any>(null); | ||
|
|
||
| const storedSidebarExpanded = localStorage.getItem('sidebar-expanded'); |
There was a problem hiding this comment.
This probably isn't the best way to do this in React.
Applies to other places too.
| } | ||
| }, [sidebarExpanded]); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
The component body is too long, we should break it up.
| import { useState } from 'react'; | ||
| import { cn } from '../../../client/cn'; | ||
|
|
||
| const CheckboxOne = () => { |
There was a problem hiding this comment.
I don't know how checkbox are done these days, but this seems like a lot.
| const FormElements = ({ user }: { user: AuthUser }) => { | ||
| useRedirectHomeUnlessUserIsAdmin({ user }); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Long component, we should break it up.
| import { useRedirectHomeUnlessUserIsAdmin } from '../../useRedirectHomeUnlessUserIsAdmin'; | ||
|
|
||
| const FormElements = ({ user }: { user: AuthUser }) => { | ||
| useRedirectHomeUnlessUserIsAdmin({ user }); |
There was a problem hiding this comment.
Perhaps it makes sense extracting this into a decorator.
Maybe it's even something Wasp should add first -lass support for.
| import { useState } from 'react'; | ||
| import { cn } from '../../../client/cn'; | ||
|
|
||
| const SwitcherTwo = () => { |
There was a problem hiding this comment.
Can we make these components names more informative (e.g., SwitcherOne, CheckboxOne). I don't know what they do. Perhaps they're different designs of the same functionality. If that's the case, we should maybe somehow extract this functionality.
| let userDelta = userCount; | ||
| let paidUserDelta = paidUserCount; |
There was a problem hiding this comment.
Doesn't seem like these guys should be lets.
|
|
||
| let userDelta = userCount; | ||
| let paidUserDelta = paidUserCount; | ||
| if (yesterdaysStats) { |
There was a problem hiding this comment.
The condition could be more explicit.
| totalRevenue = await fetchTotalLemonSqueezyRevenue(); | ||
| break; | ||
| default: | ||
| throw new Error(`Unsupported payment processor: ${paymentProcessor.id}`); |
There was a problem hiding this comment.
We should add an exhaustiveness check.
| throw new Error(`Unsupported payment processor: ${paymentProcessor.id}`); | ||
| } | ||
|
|
||
| const { totalViews, prevDayViewsChangePercent } = await getDailyPageViews(); |
There was a problem hiding this comment.
This function is getting pretty long, we should break it up.
| await context.entities.Logs.create({ | ||
| data: { | ||
| message: `Error calculating daily stats: ${error?.message}`, | ||
| level: 'job-error', |
There was a problem hiding this comment.
Double-check if these levels are enumerated somewhere.
| }; | ||
|
|
||
| let hasMore = true; | ||
| while (hasMore) { |
There was a problem hiding this comment.
This function should probably be refactored.
| let hasNextPage = true; | ||
| let currentPage = 1; | ||
|
|
||
| while (hasNextPage) { |
There was a problem hiding this comment.
Another while here (and a lot of lets), chances are we could improve it.
| // import { getDailyPageViews, getSources } from './providers/googleAnalyticsUtils'; | ||
| import { paymentProcessor } from '../payment/paymentProcessor'; | ||
|
|
||
| export type DailyStatsProps = { dailyStats?: DailyStats; weeklyStats?: DailyStats[]; isLoading?: boolean }; |
There was a problem hiding this comment.
We could improve this type, it currently allows invalid states.
| }, | ||
| }); | ||
| if (!dailyStats) { | ||
| console.log('\x1b[34mNote: No daily stats have been generated by the dailyStatsJob yet. \x1b[0m'); |
There was a problem hiding this comment.
Inline ANSI symbols look weird without a name/function describing them.
|
|
||
| async function getTotalPageViews() { | ||
| const response = await fetch( | ||
| `${PLAUSIBLE_BASE_URL}/v1/stats/aggregate?site_id=${PLAUSIBLE_SITE_ID}&metrics=pageviews`, |
There was a problem hiding this comment.
Constructing URLs like this is potentially dangerous. We should use new URL.
| if (!response.ok) { | ||
| throw new Error(`HTTP error! Status: ${response.status}`); | ||
| } | ||
| const data = (await response.json()) as PageViewsResult; |
There was a problem hiding this comment.
Unnecessary assertion.
| const headers = { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${PLAUSIBLE_API_KEY}`, | ||
| }; |
There was a problem hiding this comment.
This is repeated in several places, we should extract and specify a type for it.
|
|
||
| export async function getSources() { | ||
| const [response] = await analyticsDataClient.runReport({ | ||
| property: `properties/${PROPERTY_ID}`, |
There was a problem hiding this comment.
Possible problems here, we should validate this env var with Zod.
| ], | ||
| }); | ||
|
|
||
| let activeUsersPerReferrer: any[] = []; |
| }); | ||
| let totalViews = 0; | ||
| if (response?.rows) { | ||
| // @ts-ignore |
There was a problem hiding this comment.
I see many ts-ignores in this file.
| let viewsFromYesterday; | ||
| let viewsFromDayBeforeYesterday; |
There was a problem hiding this comment.
I see many lets in this file.
| root: 'body', | ||
| autoShow: true, | ||
| disablePageInteraction: false, | ||
| hideFromBots: import.meta.env.PROD ? true : false, // Set this to false for dev/headless tests otherwise the modal will not be visible. |
There was a problem hiding this comment.
Seems like a redundant ternary operator. But we should validate the env var with Zod.
| services: { | ||
| ga: { | ||
| label: 'Google Analytics', | ||
| onAccept: () => { |
There was a problem hiding this comment.
Is this code provided by Google or did we write it ourselves?
| <input | ||
| type='checkbox' | ||
| onChange={() => { | ||
| if (typeof setColorMode === 'function') { |
There was a problem hiding this comment.
Why is this check necessary?
| import { useEffect, useRef, useState } from 'react'; | ||
| import { cn } from '../../../client/cn'; | ||
|
|
||
| const DropdownDefault = () => { |
There was a problem hiding this comment.
The code is this component looks very familiar, I think it's duplicated somewhere else.
| const trigger = useRef<any>(null); | ||
| const dropdown = useRef<any>(null); |
There was a problem hiding this comment.
We shouldn't be using any.
| const [skip, setskip] = useState(0); | ||
| const [page, setPage] = useState(1); |
There was a problem hiding this comment.
The pagination should most likely be its own hook.
| const [email, setEmail] = useState<string | undefined>(undefined); | ||
| const [isAdminFilter, setIsAdminFilter] = useState<boolean | undefined>(undefined); |
There was a problem hiding this comment.
Better to use null than undefined.
| onChange={(e) => { | ||
| const targetValue = e.target.value === '' ? null : e.target.value; | ||
| setStatusOptions((prevValue) => { | ||
| if (prevValue?.includes(targetValue as SubscriptionStatus)) { | ||
| return prevValue?.filter((val) => val !== targetValue); | ||
| } else if (!!prevValue) { | ||
| return [...prevValue, targetValue as SubscriptionStatus]; | ||
| } else { | ||
| return prevValue; | ||
| } | ||
| }); | ||
| }} |
There was a problem hiding this comment.
This is a lot of code for an annonymous function.
There was a problem hiding this comment.
This code should be under the same mode as the server-side code used to calculate the analytics.
| const daysOfWeekArr = useMemo(() => { | ||
| if (!!weeklyStats && weeklyStats?.length > 0) { | ||
| const datesArr = weeklyStats?.map((stat) => { | ||
| // get day of week, month, and day of month | ||
| const dateArr = stat.date.toString().split(' '); | ||
| return dateArr.slice(0, 3).join(' '); | ||
| }); | ||
| return datesArr; | ||
| } | ||
| }, [weeklyStats]); |
There was a problem hiding this comment.
Many magic numbers and complex conditions in this function. We should extract variables.
| if (!!daysOfWeekArr && daysOfWeekArr?.length > 0 && !!dailyRevenueArray && dailyRevenueArray?.length > 0) { | ||
| setChartOptions({ | ||
| ...options, | ||
| xaxis: { | ||
| ...options.xaxis, | ||
| categories: daysOfWeekArr, | ||
| }, | ||
| yaxis: { | ||
| ...options.yaxis, | ||
| // get the min & max values to the neareast hundred | ||
| max: Math.ceil(Math.max(...dailyRevenueArray) / 100) * 100, | ||
| min: Math.floor(Math.min(...dailyRevenueArray) / 100) * 100, | ||
| }, | ||
| }); | ||
| } | ||
| }, [daysOfWeekArr, dailyRevenueArray]); |
There was a problem hiding this comment.
This is very complex for an annonymous function.
| }[]; | ||
| } | ||
|
|
||
| const RevenueAndProfitChart = ({ weeklyStats, isLoading }: DailyStatsProps) => { |
There was a problem hiding this comment.
This component is pretty long, we should break it up.
| @@ -0,0 +1,47 @@ | |||
| import { type PageViewSource } from 'wasp/entities'; | |||
|
|
|||
| const SourcesTable = ({ sources }: { sources: PageViewSource[] | undefined }) => { | |||
There was a problem hiding this comment.
Why leave the option of passing in undefined?
| type PageViewsStats = { | ||
| totalPageViews: number | undefined; | ||
| prevDayViewsChangePercent: string | undefined; | ||
| }; |
There was a problem hiding this comment.
This type is imprecise, it allows invalid states.
| const isDeltaPositive = useMemo(() => { | ||
| return !!dailyStats?.paidUserDelta && dailyStats?.paidUserDelta > 0; | ||
| }, [dailyStats]); |
There was a problem hiding this comment.
I don't think this useMemo is necessary.
| const isDeltaPositive = useMemo(() => { | ||
| if (!weeklyStats) return false; | ||
| return (weeklyStats[0].totalRevenue - weeklyStats[1]?.totalRevenue) > 0; | ||
| }, [weeklyStats]); |
There was a problem hiding this comment.
There's some repetitino here, we shuold look into that.
| const deltaPercentage = useMemo(() => { | ||
| if ( !weeklyStats || weeklyStats.length < 2 || isLoading) return; | ||
| if ( weeklyStats[1]?.totalRevenue === 0 || weeklyStats[0]?.totalRevenue === 0 ) return 0; | ||
|
|
||
| weeklyStats.sort((a, b) => b.id - a.id); | ||
|
|
||
| const percentage = ((weeklyStats[0].totalRevenue - weeklyStats[1]?.totalRevenue) / weeklyStats[1]?.totalRevenue) * 100; | ||
| return Math.floor(percentage); | ||
| }, [weeklyStats]); |
There was a problem hiding this comment.
A lot of code for an annonymous function.
| const isDeltaPositive = useMemo(() => { | ||
| return !!dailyStats?.userDelta && dailyStats.userDelta > 0; | ||
| }, [dailyStats]); |
There was a problem hiding this comment.
More repetition here, we should look into that.
| <VerifyEmailForm /> | ||
| <br /> | ||
| <span className='text-sm font-medium text-gray-900'> | ||
| If everything is okay, <WaspRouterLink to={routes.LoginRoute.to} className='underline'>go to login</WaspRouterLink> |
There was a problem hiding this comment.
Can we somehow detect whether everything's ok?
Could we log the user in automatically without telling them to do it themselves?
| <ResetPasswordForm /> | ||
| <br /> | ||
| <span className='text-sm font-medium text-gray-900'> | ||
| If everything is okay, <WaspRouterLink to={routes.LoginRoute.to}>go to login</WaspRouterLink> |
There was a problem hiding this comment.
Same questions I had for the EmailVerificationPage.
| username: (data: any) => data.email, | ||
| isAdmin: (data: any) => adminEmails.includes(data.email), | ||
| email: (data: any) => data.email, |
| ContentType: `${fileType}`, | ||
| }; | ||
| const command = new PutObjectCommand(s3Params); | ||
| const uploadUrl = await getSignedUrl(s3Client, command, { expiresIn: 3600,}); |
There was a problem hiding this comment.
What time is 3600 here? We should document it.
I'm guessing it's an hour. Why so long?
| Key: key, | ||
| }; | ||
| const command = new GetObjectCommand(s3Params); | ||
| return await getSignedUrl(s3Client, command, { expiresIn: 3600 }); |
| return { uploadUrl, key: Key }; | ||
| } | ||
|
|
||
| export const getDownloadFileSignedURLFromS3 = async ({ key }: { key: string }) => { |
There was a problem hiding this comment.
Missing return type.
| userInfo: string; | ||
| } | ||
|
|
||
| export const getUploadFileSignedURLFromS3 = async ({fileType, userInfo}: S3Upload) => { |
There was a problem hiding this comment.
Missing return type (I think this applies to a lot of code).
| useEffect(() => { | ||
| allUserFiles.refetch(); | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (fileKeyForS3.length > 0) { | ||
| refetchDownloadUrl() | ||
| .then((urlQuery) => { | ||
| switch (urlQuery.status) { | ||
| case 'error': | ||
| console.error('Error fetching download URL', urlQuery.error); | ||
| alert('Error fetching download'); | ||
| return; | ||
| case 'success': | ||
| window.open(urlQuery.data, '_blank'); | ||
| return; | ||
| } | ||
| }) | ||
| .finally(() => { | ||
| setFileKeyForS3(''); | ||
| }); | ||
| } | ||
| }, [fileKeyForS3]); |
There was a problem hiding this comment.
Unnamed useEffects, missing exhaustiveness check, possibility for early returns, we should kick out then and finally in favor of async/await.
| } | ||
|
|
||
| await uploadFileWithProgress({ file, setUploadProgressPercent }); | ||
| formElement.reset(); |
There was a problem hiding this comment.
Weird stuff happening with the form here, we should investigate.
| <h2 className='text-xl font-bold'>Uploaded Files</h2> | ||
| {allUserFiles.isLoading && <p>Loading...</p>} | ||
| {allUserFiles.error && <p>Error: {allUserFiles.error.message}</p>} | ||
| {!!allUserFiles.data && allUserFiles.data.length > 0 && !allUserFiles.isLoading ? ( | ||
| allUserFiles.data.map((file: File) => ( | ||
| <div | ||
| key={file.key} | ||
| className={cn( | ||
| 'flex flex-col sm:flex-row items-start sm:items-center justify-between gap-3', | ||
| { | ||
| 'opacity-70': file.key === fileKeyForS3 && isDownloadUrlLoading, | ||
| } | ||
| )} | ||
| > | ||
| <p>{file.name}</p> | ||
| <button | ||
| onClick={() => setFileKeyForS3(file.key)} | ||
| disabled={file.key === fileKeyForS3 && isDownloadUrlLoading} | ||
| className='min-w-[7rem] text-sm text-gray-800/90 bg-purple-50 shadow-md ring-1 ring-inset ring-slate-200 py-1 px-2 rounded-md hover:bg-purple-100 duration-200 ease-in-out focus:outline-none focus:shadow-none hover:shadow-none disabled:cursor-not-allowed' | ||
| > | ||
| {file.key === fileKeyForS3 && isDownloadUrlLoading ? 'Loading...' : 'Download'} | ||
| </button> | ||
| </div> | ||
| )) |
There was a problem hiding this comment.
The component is very complex, we should break it up.
| 'video/mp4', | ||
| ]; | ||
|
|
||
| export async function uploadFileWithProgress({ file, setUploadProgressPercent }: FileUploadProgress) { |
There was a problem hiding this comment.
Could this be a hook that handles everything?
|
|
||
| const { uploadUrl, key } = await getUploadFileSignedURLFromS3({ fileType, userInfo }); | ||
|
|
||
| return await context.entities.File.create({ |
There was a problem hiding this comment.
Should we validate the payload?
No description provided.